Technically the refactor looks very good!

There are two things that potentially will be raised:

1. The repo is very document heavy, there are several places where the refactor has deleted comments all together or at least omitted details/reformulated with a slightly different meaning. I'll reference a version that partly restores comments and will also add a comment that diffs old and new comments where things have changed
2. I like that this PR is also addressing Separate roles for contract owner and workload ID permissioner #49 and Abstract the TEE policy verification logic into a library useable by external contracts #52. But it makes the review process more difficult as even more things have moved.

Below are the methods whose doc blocks changed meaningfully. For each: 1) old, 2) new

initialize

1. Old (IBlockBuilderPolicy)

/// @notice Initializer to set the FlashtestationRegistry contract which verifies TEE quotes and the initial owner of the contract
/// @param _initialOwner The address of the initial owner of the contract
/// @param _registry The address of the registry contract

2. New (IBlockBuilderPolicy)

/// @notice Initializer to set the FlashtestationRegistry contract which verifies TEE quotes and the initial owner of the contract
/// @param _initialOwner The address of the initial owner of the contract
/// @param _registry The address of the registry contract
/// @param _workloadDeriver Address of the workload deriver used for workloadId computation

isAllowedPolicy

1. Old (IBlockBuilderPolicy)

/// @notice Check if this TEE-controlled address has registered a valid TEE workload with the registry, and
/// if the workload is approved under this policy
/// @param teeAddress The TEE-controlled address
/// @return allowed True if the TEE is using an approved workload in the policy
/// @return workloadId The workloadId of the TEE that is using an approved workload in the policy, or 0 if
/// the TEE is not using an approved workload in the policy

2. New (IBasePolicy + BasePolicy inline)

/// @notice Check if this TEE-controlled address has a valid registry registration and
/// whether its workload is approved under this policy.
/// @param teeAddress The TEE-controlled address.
/// @return allowed True if the TEE is using an approved workload in the policy.
/// @return workloadId The workloadId of the TEE that is using an approved workload, or 0 if not allowed.

_cachedIsAllowedPolicy

1. Old (BlockBuilderPolicy)

/// @notice isAllowedPolicy but with caching to reduce gas costs
/// @dev This function is only used by the verifyBlockBuilderProof function, which needs to be as efficient as possible
/// because it is called onchain for every flashblock. The workloadId is cached to avoid expensive recomputation
/// @dev A careful reader will notice that this function does not delete stale cache entries. It overwrites them
/// if the underlying TEE registration is still valid. But for stale cache entries in every other scenario, the
/// cache entry persists indefinitely. This is because every other instance results in a return value of (false, 0)
/// to the caller (which is always the verifyBlockBuilderProof function) and it immediately reverts. This is an unfortunate
/// consequence of our need to make this function as gas-efficient as possible, otherwise we would try to cleanup
/// stale cache entries
/// @param teeAddress The TEE-controlled address
/// @return True if the TEE is using an approved workload in the policy
/// @return The workloadId of the TEE that is using an approved workload in the policy, or 0 if
/// the TEE is not using an approved workload in the policy

2. New (BasePolicy)

/// @notice Cached variant of isAllowedPolicy for gas-sensitive call paths.
/// @dev Why this exists:
/// - isAllowedPolicy is view but can be expensive because workloadId derivation can be costly.
/// - Some policies (notably BlockBuilderPolicy) need an O(1)-ish authorization check on the hot path
/// (e.g. verifyBlockBuilderProof), so we cache the derived workloadId keyed by the TEE's quoteHash.
///
/// @dev How it is expected to be used:
/// - Derived policies call _cachedIsAllowedPolicy(teeAddress) from their hot path.
/// - Cache storage lives in the derived policy; BasePolicy only implements the algorithm and calls the
/// _getCachedWorkload / _setCachedWorkload hooks.
/// - The function checks registry validity + quoteHash; on cache hit it avoids recomputing derivation.
/// - On cache miss (or quote change), it falls back to isAllowedPolicy and updates the cache if allowed.
///
/// @dev Cache cleanup:
/// - This function does not proactively delete stale cache entries. On invalid registrations it returns
/// (false, 0) and callers typically revert, so cleanup is intentionally skipped to keep the hot path cheap.
/// - Stale cache entries for all other "not allowed" scenarios persist indefinitely for the same reason:
/// the caller reverts immediately and the hot path avoids extra storage writes.
///
/// @param teeAddress The TEE-controlled address to check.
/// @return allowed True if the TEE is registered, valid, and running an approved workload.
/// @return workloadId The approved workloadId for this TEE (or 0 if not allowed).

workloadIdForTDRegistration

1. Old (IBlockBuilderPolicy + inline)

/// @notice Application specific mapping of registration data to a workload identifier
/// @dev Think of the workload identifier as the version of the application for governance.
/// The workloadId verifiably maps to a version of source code that builds the TEE VM image
/// @param registration The registration data from a TEE device
/// @return workloadId The computed workload identifier

Inline:

// We expect FPU and SSE xfam bits to be set, and anything else should be handled by explicitly allowing the workloadid
// We don't mind VE_DISABLED, PKS, and KL tdattributes bits being set either way, anything else requires explicitly allowing the workloadid

2. New (IBlockBuilderPolicy + BlockBuilderPolicy)

/// @notice Application specific mapping of registration data to a workload identifier
/// @dev Think of the workload identifier as the version of the application for governance.
/// The workloadId verifiably maps to a version of source code that builds the TEE VM image
/// @param registration The registration data from a TEE device
/// @return workloadId The computed workload identifier

Inline:

// Uses TDXWorkloadDeriverLib (see the deriver for constants and derivation details).

addWorkloadToPolicy

1. Old (IBlockBuilderPolicy)

/// @notice Add a workload to a policy (governance only)
/// @notice Only the owner of this contract can add workloads to the policy
/// and it is the responsibility of the owner to ensure that the workload is valid
/// otherwise the address associated with this workload has full power to do anything
/// who's authorization is based on this policy
/// @dev The commitHash solves the following problem; ...
/// @param workloadId The workload identifier
/// @param commitHash The 40-character hexadecimal commit hash of the git repository
/// whose source code is used to build the TEE image identified by the workloadId
/// @param sourceLocators An array of URIs pointing to the source code

2. New (IBasePolicy)

/// @notice Add a workload to a policy (governance only).
/// @notice Only the policy authority can add workloads to the policy, and it is the responsibility of the
/// authority to ensure that the workload is valid; otherwise the address associated with this workload has
/// full power to do anything whose authorization is based on this policy.
/// @dev The commitHash solves the following problem; ...
/// @param workloadId The workload identifier.
/// @param commitHash The 40-character hexadecimal commit hash ...
/// @param sourceLocators An array of URIs pointing to the source code.

removeWorkloadFromPolicy

1. Old (IBlockBuilderPolicy)

/// @notice Remove a workload from a policy (governance only)
/// @param workloadId The workload identifier

2. New (IBasePolicy)

/// @notice Remove a workload from a policy (governance only).

getWorkloadMetadata

1. Old (IBlockBuilderPolicy)

/// @notice Mapping from workloadId to its metadata (commit hash and source locators)
/// @dev This is only updateable by governance (i.e. the owner) of the Policy contract
/// Adding and removing a workload is O(1)
/// @param workloadId The workload identifier to query
/// @return The metadata associated with the workload

2. New (IBasePolicy)

/// @notice Get metadata for an approved workload.
/// @dev This is only updateable by governance (i.e. the policy authority). Adding and removing a workload is O(1).
/// @param workloadId The workload identifier to query.
/// @return The metadata associated with the workload.

#56

Thank you for the feedback Frieder. I merged in your doc changes.

Regarding the other issues this PR addresses, this was a happy accident. Due to the requirement that the storage layout remain unchanged, BasePolicy cannot inherit from OwnableUpgradeable to access the owner state directly. Instead, it uses an abstract hook pattern (_checkPolicyAuthority()) that BlockBuilderPolicy implements by calling _checkOwner(). Similarly, before the tdx policy verification logic was BlockBuilderPolicy, but we decided to make derivers that needed this logic, and I wouldn't want a deriver to inherit from BlockBuilderPolicy so it just made sense to refactor out.

@Melvillian do you think you could take a look and let me know what you think?